fix: improve error handling for partial message control flow (fixes #7312) #7315
Conversation
- Introduce PartialMessageControlFlowError custom error class - Replace confusing "Current ask promise was ignored" errors with descriptive messages - These errors occur during normal operation when handling partial messages - The errors are caught by callers but the messages were confusing AI models - Fixes issue #7312 where tool calls appeared to be failing when they were actually working as expected
dosubot
bot
added
size:S
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| import { AutoApprovalHandler } from "./AutoApprovalHandler" | ||
|
|
||
| // Custom error class for expected control flow in partial message handling | ||
| class PartialMessageControlFlowError extends Error { |
There was a problem hiding this comment.
Consider exporting this error class for use in other modules that might need to distinguish between expected control flow and actual errors. This would improve error handling consistency across the codebase.
| import { restoreTodoListForTask } from "../tools/updateTodoListTool" | ||
| import { AutoApprovalHandler } from "./AutoApprovalHandler" | ||
|
|
||
| // Custom error class for expected control flow in partial message handling |
There was a problem hiding this comment.
Consider adding JSDoc documentation to this class to make it more discoverable and explain when it should be used vs regular errors:
| // Custom error class for expected control flow in partial message handling | |
| // Custom error class for expected control flow in partial message handling | |
| /** | |
| * Error class for expected control flow behaviors during partial message handling. | |
| * These are not actual errors but normal control flow that uses Promise rejection | |
| * for flow control. Callers typically handle these with .catch(() => {}). | |
| */ | |
| class PartialMessageControlFlowError extends Error { |
| constructor(message: string) { | ||
| super(message) | ||
| this.name = "PartialMessageControlFlowError" | ||
| } |
There was a problem hiding this comment.
The error messages use slightly different formats. Consider standardizing them for consistency. Also, instead of hardcoding messages in three places, consider using static methods or constants:
| } | |
| class PartialMessageControlFlowError extends Error { | |
| static readonly PARTIAL_UPDATE = "Partial message update - expected behavior" | |
| static readonly NEW_PARTIAL = "New partial message - expected behavior" | |
| static readonly PROMISE_SUPERSEDED = "Ask promise superseded - expected behavior" | |
| constructor(message: string) { | |
| super(message) | |
| this.name = "PartialMessageControlFlowError" | |
| } | |
| } |
Then use like: throw new PartialMessageControlFlowError(PartialMessageControlFlowError.PARTIAL_UPDATE)
| throw new Error("Current ask promise was ignored (#1)") | ||
| // This is expected behavior during partial message updates | ||
| // Return early instead of throwing to avoid confusing error logs | ||
| throw new PartialMessageControlFlowError("Partial message update - expected behavior") |
There was a problem hiding this comment.
Good use of descriptive error messages that clearly indicate this is expected behavior. This will help both developers and AI models understand these aren't actual failures.
| throw new Error("Current ask promise was ignored") | ||
| // This can happen when multiple asks are sent in sequence | ||
| // It's a normal part of the control flow, not an error | ||
| throw new PartialMessageControlFlowError("Ask promise superseded - expected behavior") |
There was a problem hiding this comment.
While the PR mentions all tests pass, it would be good to add specific test coverage for this error handling to ensure callers using .catch(() => {}) continue to work as expected with the new error class.
hannesrudolph
added
the
Issue/PR - Triage
|
This doesn't seem to solve the issue |
Summary
This PR addresses issue #7312 where tool calls appeared to be failing when they were actually working as expected. The root cause was that expected control flow behaviors during partial message handling were throwing generic errors with confusing messages.
Problem
As @Mushoz correctly pointed out, the root issue wasn't just confusing error messages - it was that tool calls appeared to be failing. Investigation revealed that these "failures" were actually expected control flow behaviors being misreported as errors.
When handling partial messages during streaming, the Task class was throwing errors with the message "Current ask promise was ignored". While these errors were being caught by callers (using
.catch(() => {})), the error messages were visible to AI models and causing confusion, leading to unnecessary retries.Solution
Introduced a custom
PartialMessageControlFlowErrorclass to differentiate between expected control flow and actual errors. The new error messages clearly indicate these are expected behaviors:These errors are still thrown (as Promise rejections) to maintain the existing control flow, but now have clearer messages that won't confuse AI models or developers.
Changes
PartialMessageControlFlowErrorcustom error classsrc/core/task/Task.tsat three locations where "Current ask promise was ignored" errors were thrownTesting
npx vitest run core/task/__tests__/Task.spec.ts)npx vitest run core/tools/__tests__)Impact
This change ensures that:
Related Issues
Fixes #7312
Notes
This issue was previously addressed for other models but resurfaced for gpt-oss-120b and potentially other OpenAI-compatible models. The solution properly categorizes expected control flow behaviors to reduce noise in error logs and help identify actual tool failures when they occur.
Important
Introduces
PartialMessageControlFlowErrorinTask.tsto improve error handling for partial message control flow, clarifying expected behaviors and reducing confusion.PartialMessageControlFlowErrorinTask.tsto handle expected control flow during partial message handling.Task.tsat three locations where "Current ask promise was ignored" errors were thrown.This description was created by
for c515f81. You can customize this summary. It will automatically update as commits are pushed.